feat: v3 invoice api get review - #4573
Merged
tothandras merged 2 commits intoJun 25, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBilling charge, customer, and invoice contracts are renamed across the API spec, generated client, Go handlers, and e2e coverage. External IDs become external references, and lifecycle ownership shifts to ChangesBilling API contract alignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR updates the v3 invoice and charge API contract. The main changes are:
Confidence Score: 4/5This is close, but the invoice workflow reference should be fixed before merging.
api/v3/handlers/billinginvoices/convert.go Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
api/v3/handlers/billinginvoices/convert.go:183-186
**Empty Profile Reference**
When an older or migrated invoice has no `SourceBillingProfileID`, this now returns `source_billing_profile: {"id":""}` because the new response shape makes the profile reference object required. That gives callers a present billing-profile reference with an invalid ULID instead of failing conversion or omitting a nullable snapshot field, so clients that parse the reference as a real profile ID can reject otherwise readable invoices.
Reviews (6): Last reviewed commit: "fix: review comment" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
e2e/billinginvoices_v3_test.go (1)
221-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert one of the renamed invoice fields here too.
This subtest currently proves the union accessor rename, but none of the contract fields changed in this PR are actually exercised. Adding a couple of checks on
workflow.source_billing_profile.idand the first line’slifecycle_controllerwould make this test protect the converter changes, not just the renamed decoder.Suggested assertions
stdInv, err := inv.AsBillingInvoiceStandard() require.NoError(t, err, "expected invoice to decode as BillingInvoiceStandard") assert.Equal(t, invoiceID, stdInv.Id) assert.Equal(t, apiv3.BillingInvoiceStandardTypeStandard, stdInv.Type) assert.Equal(t, apiv3.CurrencyCode("USD"), stdInv.Currency) assert.NotEmpty(t, stdInv.Status) assert.NotEmpty(t, stdInv.CreatedAt) + assert.NotEmpty(t, stdInv.Workflow.SourceBillingProfile.Id) + if assert.NotNil(t, stdInv.Lines) && assert.NotEmpty(t, *stdInv.Lines) { + assert.NotEmpty(t, (*stdInv.Lines)[0].LifecycleController) + } })As per path instructions, "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/billinginvoices_v3_test.go` around lines 221 - 237, The v3 invoice GET test only verifies the union accessor rename, so it does not cover the renamed contract fields changed in this PR. In the `Should return the invoice via v3 GET` subtest, add assertions against the decoded `BillingInvoiceStandard` (`stdInv`) for `workflow.source_billing_profile.id` and the first line’s `lifecycle_controller` so the converter changes are exercised as well. Use the existing `AsBillingInvoiceStandard` decode path and assert the expected values alongside the current `Id`, `Type`, `Currency`, `Status`, and `CreatedAt` checks.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/spec/packages/aip-client-javascript/src/models/schemas.ts`:
- Around line 4468-4501: The runtime schema is missing the credit-note
discriminator variant, so fix the TypeSpec/generation source for invoice schemas
rather than editing generated output in aip-client-javascript. Update the
credit-note model behind invoiceCreditNote so its type is pinned to
z.literal('credit_note') instead of the broad invoiceType, and ensure the
top-level invoice discriminated union includes that credit-note variant
alongside invoiceStandard. After updating the source definition, regenerate the
client so invoice.parse(...) accepts real credit_note payloads and the
standalone schema has the correct discriminator.
In `@api/spec/packages/aip-client-javascript/src/models/types.ts`:
- Around line 2977-3009: The InvoiceCreditNote model has an incorrect
discriminator in the generated types, since the InvoiceCreditNote interface
currently uses the wrong type literal and should represent the credit note
variant instead. Update the underlying TypeSpec source for the InvoiceCreditNote
discriminator (not this generated types.ts file) so the model narrows correctly,
then regenerate the client artifacts to pick up the fix in the generated
InvoiceCreditNote definition.
In `@api/spec/packages/aip/src/invoices/invoice.tsp`:
- Around line 787-789: The summary for external_references is still using the
old “External IDs” label, so update the `@summary` on
InvoiceLineExternalReferences to match the renamed field and use terminology
consistent with the current API. Locate the external_references declaration in
the invoice model and change the summary text so generated docs and users see
the correct field name and meaning.
- Around line 295-304: The discriminator on InvoiceCreditNote is currently wired
to InvoiceType, but that enum only represents the standard invoice value, so the
model cannot correctly identify credit notes. Update the InvoiceCreditNote type
field to use the correct credit-note discriminator shape/value, and if needed
adjust InvoiceType or introduce a dedicated enum so the contract matches the
actual implementation and generated clients serialize the right type.
In `@api/v3/handlers/billingprofiles/convert.go`:
- Around line 162-168: The supplier snapshot conversion is being routed through
ToAPIBillingParty, but that helper only populates Addresses when
supplier.Address.Country is set, which drops line1/city/state/phone-only data.
Update ToAPIBillingParty (and any related address mapping it uses) so it emits
Addresses whenever any supplier address field is present, then keep
BillingSupplier conversion using that helper only if it preserves all address
fields.
---
Nitpick comments:
In `@e2e/billinginvoices_v3_test.go`:
- Around line 221-237: The v3 invoice GET test only verifies the union accessor
rename, so it does not cover the renamed contract fields changed in this PR. In
the `Should return the invoice via v3 GET` subtest, add assertions against the
decoded `BillingInvoiceStandard` (`stdInv`) for
`workflow.source_billing_profile.id` and the first line’s `lifecycle_controller`
so the converter changes are exercised as well. Use the existing
`AsBillingInvoiceStandard` decode path and assert the expected values alongside
the current `Id`, `Type`, `Currency`, `Status`, and `CreatedAt` checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f73281d2-7412-43e0-86cc-41c0f3ccb9ae
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (17)
.agents/skills/api/rules/aip-126-enums.mdapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/operations/customers.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/customers/charges/charges.tspapi/spec/packages/aip/src/customers/customer.tspapi/spec/packages/aip/src/invoices/invoice.tspapi/spec/packages/aip/src/invoices/operations.tspapi/spec/packages/aip/src/shared/enums.tspapi/v3/api.gen.goapi/v3/handlers/billinginvoices/convert.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/charges/convert.goapi/v3/handlers/customers/charges/create.goapi/v3/handlers/customers/convert.gen.goe2e/billinginvoices_v3_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/spec/packages/aip/src/invoices/invoice.tsp (1)
33-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the union docs aligned with the exposed variants.
credit_noteis still listed in the publicInvoicedescription, but the union member is commented out for now. That can make generated docs imply clients may receive a variant the schema doesn’t allow yet.Suggested tweak
- * - `standard`: a standard invoice for charges owed. - * - `credit_note`: a credit note reducing a previous invoice amount. + * - `standard`: a standard invoice for charges owed.As per path instructions, "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/spec/packages/aip/src/invoices/invoice.tsp` around lines 33 - 34, The public Invoice union docs still mention a variant that is not currently exposed, so the description is out of sync with the actual schema. Update the documentation near the Invoice type in invoice.tsp to match only the active union members, and keep any mention of credit_note aligned with whether the variant is actually declared in the Invoice union or removed from the docs until it is re-enabled.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api/spec/packages/aip/src/invoices/invoice.tsp`:
- Around line 33-34: The public Invoice union docs still mention a variant that
is not currently exposed, so the description is out of sync with the actual
schema. Update the documentation near the Invoice type in invoice.tsp to match
only the active union members, and keep any mention of credit_note aligned with
whether the variant is actually declared in the Invoice union or removed from
the docs until it is re-enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebe6b2e3-b594-4fb3-a94d-95342abaf9c1
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (6)
api/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/invoices/invoice.tspapi/v3/api.gen.goapi/v3/handlers/billinginvoices/convert.go
💤 Files with no reviewable changes (1)
- api/spec/packages/aip-client-javascript/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- api/v3/handlers/billinginvoices/convert.go
- api/spec/packages/aip-client-javascript/src/models/schemas.ts
- api/spec/packages/aip-client-javascript/src/models/types.ts
tothandras
force-pushed
the
feat/v3-invoice-api-get-review
branch
from
June 25, 2026 19:17
658c940 to
7c9ff4e
Compare
tothandras
force-pushed
the
feat/v3-invoice-api-get-review
branch
from
June 25, 2026 19:47
7c9ff4e to
6c98216
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/spec/packages/aip-client-javascript/src/models/types.ts`:
- Around line 1446-1447: The generated JSDoc for the renamed external_references
fields still says “External identifiers,” so update the corresponding TypeSpec
comments to use “external references” and regenerate the aip-client-javascript
types instead of editing types.ts directly. Make this change for the affected
model definitions that feed the external_references properties, then re-run
generation so all listed occurrences pick up the new wording consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cbcb0527-3b49-4b82-8220-18bc0e94d8bf
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (17)
.agents/skills/api/rules/aip-126-enums.mdapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/operations/customers.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/customers/charges/charges.tspapi/spec/packages/aip/src/customers/customer.tspapi/spec/packages/aip/src/invoices/invoice.tspapi/spec/packages/aip/src/invoices/operations.tspapi/spec/packages/aip/src/shared/enums.tspapi/v3/api.gen.goapi/v3/handlers/billinginvoices/convert.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/charges/convert.goapi/v3/handlers/customers/charges/create.goapi/v3/handlers/customers/convert.gen.goe2e/billinginvoices_v3_test.go
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/api/rules/aip-126-enums.md
- api/spec/packages/aip-client-javascript/src/models/operations/customers.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- api/spec/packages/aip/src/customers/customer.tsp
- api/v3/handlers/customers/charges/create.go
- e2e/billinginvoices_v3_test.go
- api/spec/packages/aip/src/invoices/operations.tsp
- api/v3/handlers/billingprofiles/convert.go
- api/v3/handlers/customers/convert.gen.go
- api/spec/packages/aip/src/shared/enums.tsp
- api/v3/handlers/billinginvoices/convert.go
- api/v3/handlers/customers/charges/convert.go
- api/spec/packages/aip-client-javascript/src/index.ts
- api/spec/packages/aip/src/customers/charges/charges.tsp
- api/spec/packages/aip-client-javascript/src/models/schemas.ts
- api/spec/packages/aip/src/invoices/invoice.tsp
turip
approved these changes
Jun 25, 2026
turip
left a comment
turip
left a comment
Member
There was a problem hiding this comment.
Checked the LifecycleController handler conversions. The mappings are consistent with the public contract: manual stays manual, and both system/subscription-owned domain resources surface as system. Looks good.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Charge*/Invoice*naming pluslifecycle_controller.external_references(including payment/invoicing context).external_ids→external_references,managed_by→lifecycle_controller,billingAddress→address) and invoice model restructuring.unknownenum member guidance is now a recommendation.